Repository navigation
Fix #11168 FP returnDanglingLifetime (local iterator points at global struct) - #8979
chrchr-github wants to merge 2 commits into
Conversation
dmcppcheck
left a comment
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
This PR causes false negatives when a member is an array. The new condition stops following the lifetime at . whenever the top token is a unary * and the right side of . has a varId. It never checks that the member is a pointer, so it also applies to array members. In that case *s.a is part of the local s. Real dangling returns such as return &*s.a;, return *s.a; (returning int&) and return &*s.b[0]; are now no longer reported; base cppcheck reports all three. The FP from #11168 (&*x.a where a is a pointer) is fixed.
A safer check would test that the member really is a pointer and not an array, for example vt->pointer > 0 together with !variable()->isArray(). Tests for the array case should be added to keep this behaviour from breaking again.
Testing
I built cppcheck and testrunner for both pr/ and base/. ./testrunner TestAutoVariables passes, and so does the full ./testrunner (5374 tests, 0 failures).
Test file scratch/a.cpp:
struct S { int a[10]; int b[3][3]; int* p; };
int* f1() { S s; return &*s.a; }
int* f2() { S s{}; return &*s.b[0]; }
int& f3() { S s{}; return *s.a; }
int* f4() { S s{}; return &s.a[0]; }
int* g1() { S s; return &*s.p; }Command: cppcheck -q --enable=all a.cpp
| line | base | pr |
|---|---|---|
f1 &*s.a (array) |
returnDanglingLifetime (correct) | nothing (FN) |
f2 &*s.b[0] (2D array) |
returnDanglingLifetime (correct) | nothing (FN) |
f3 *s.a returned as int& |
returnReference (correct) | nothing (FN) |
f4 &s.a[0] |
returnDanglingLifetime | returnDanglingLifetime |
g1 &*s.p (pointer) |
returnDanglingLifetime (FP) | nothing (fixed) |
Iterator members (return *t.it; / &*t.it) are no longer reported either. That looks correct, because the iterator may point into some other container.
Automated review of cf0a3b9 by Claude. The pull request was built and tested locally.
| else if (vartok->str() == ".") { | ||
| if (!Token::simpleMatch(vartok->astOperand1(), ".") && | ||
| !(vartok->astOperand2() && vartok->astOperand2()->valueType() && vartok->astOperand2()->valueType()->reference != Reference::None)) | ||
| !(vartok->astOperand2() && ((vartok->astOperand2()->varId() && tok->isUnaryOp("*")) || (vartok->astOperand2()->valueType() && vartok->astOperand2()->valueType()->reference != Reference::None)))) |
There was a problem hiding this comment.
AI review
varId() && tok->isUnaryOp("*") is true for array members too, and there dereferencing gives an element of the local object. Example: struct S { int a[10]; }; int* f() { S s; return &*s.a; } and int& f() { S s{}; return *s.a; }. Both were reported before (returnDanglingLifetime / returnReference) and are silent with this PR. The same happens for &*s.b[0] with int b[3][3]. Consider checking that the member is a real pointer, e.g. vt && vt->pointer > 0 && !(var && var->isArray()), and add tests for the array cases.
No description provided.